Add guard against getEntriesByType #553
Conversation
| @@ -15,7 +15,11 @@ | |||
| */ | |||
|
|
|||
| export const getNavigationEntry = (): PerformanceNavigationTiming | void => { | |||
| const navigationEntry = performance.getEntriesByType('navigation')[0]; | |||
| // JSDOM does not implement getEntriesByType: https://github.com/jsdom/jsdom/issues/3309 | |||
There was a problem hiding this comment.
Feels sad to accommodate an inadequacy of a test-env-specific library, given that this API is supported in all major browsers: https://caniuse.com/mdn-api_performance_getentriesbytype
There was a problem hiding this comment.
I agree. However it's a guard, rather than a polyfill, or holding ourselves back in using new features or syntax. So I'm comfortable with this. We've a few cases of this in the library.
But let's see if @philipwalton agrees!
There was a problem hiding this comment.
Optional chaining could make this not so bad, but in general if you're using jsdom for testing, you'll be mocking out a bunch of stuff anyways and it shouldn't generally be put on the library to do that.
Nothing will work in jsdom anyways, looks like there's no PerformanceObservers either, so I wonder if the tester should just short circuit it completely.
There was a problem hiding this comment.
For those we wrap it in a try/catch anyway:
https://github.com/GoogleChrome/web-vitals/blob/v5/src/lib/observe.ts
Hence why they didn’t cause a problem.
For TTFB however we just call it as it doesn’t need a PerformanceObserver.
I don’t think the intent is to test the library in JSDOM, it’s more the whole page is being loaded in that (including this script) and it hits this error.
There was a problem hiding this comment.
Oh and I tried optional chaining initially and still complained getEntriesByTyoe wasn’t a function. Because performance does exist but not much under it. Open to ideas on how to implement this better if you know some better optional chaining syntax for that? For now I just reverted back to what we had in v4.
There was a problem hiding this comment.
I don’t think the intent is to test the library in JSDOM, it’s more the whole page is being loaded in that (including this script) and it hits this error.
right, but I guarantee this isn't the only thing broken in jsdom and they already have a "set up the mocks" section somewhere because there's always something (usually several things) that need to be mocked out
Open to ideas on how to implement this better if you know some better optional chaining syntax for that?
performance.getEntriesByType?.('navigation')[0] should work
There was a problem hiding this comment.
Oh that worked! Thank you. Updated in 16f4a51
|
Closing after discussion off line and finding the fix in the offending code that depended on this. |
Causing failures when testing Google upgrades.